-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[OpenMP] Add test to print interop identifiers #161434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-offload Author: Jan Patrick Lehr (jplehr) ChangesThe test covers some of the identifier symbols in the interop runtime. This test, for now, is to guard against complete breakage, which was the result of the other Depends on #161429 to restore functionality. Full diff: https://github.com/llvm/llvm-project/pull/161434.diff 1 Files Affected:
diff --git a/offload/test/offloading/interop-print.c b/offload/test/offloading/interop-print.c
new file mode 100644
index 0000000000000..a474501649777
--- /dev/null
+++ b/offload/test/offloading/interop-print.c
@@ -0,0 +1,74 @@
+// RUN: %libomptarget-compile-amdgcn-amd-amdhsa
+// RUN: %libomptarget-run-generic 2>&1 | \
+// RUN: %fcheck-generic -check-prefix=AMDCHECK
+
+// REQUIRES: amdgpu
+
+#include <omp.h>
+#include <stdio.h>
+
+const char* interop_int_to_string( const int interop_int )
+{
+ switch( interop_int )
+ {
+ case 1:
+ return "cuda";
+ case 2:
+ return "cuda_driver";
+ case 3:
+ return "opencl";
+ case 4:
+ return "sycl";
+ case 5:
+ return "hip";
+ case 6:
+ return "level_zero";
+ case 7:
+ return "hsa";
+ default:
+ return "unknown";
+ }
+}
+
+int main( int argc, char** argv )
+{
+ omp_interop_t iobj = omp_interop_none;
+ #pragma omp interop init(targetsync: iobj)
+
+ int err;
+ int interop_int = omp_get_interop_int( iobj, omp_ipr_fr_id, &err );
+
+ if( err )
+ {
+ fprintf( stderr, "omp_get_interop_int failed: %d\n", err );
+ return -1;
+ }
+
+ // FIXME: This should be hsa instead of hip
+ // AMDCHECK: {{.*}} hip
+ printf( "omp_get_interop_int returned %s\n", interop_int_to_string( interop_int ) );
+
+ const char* interop_vendor = omp_get_interop_str( iobj, omp_ipr_vendor_name, &err );
+ if( err )
+ {
+ fprintf( stderr, "omp_get_interop_str failed: %d\n", err );
+ return -1;
+ }
+
+ // AMDCHECK: {{.*}} amd
+ printf( "omp_get_interop_str returned %s\n", interop_vendor );
+
+ const char* interop_fr_name = omp_get_interop_str( iobj, omp_ipr_fr_name, &err );
+ if( err )
+ {
+ fprintf( stderr, "omp_get_interop_str failed: %d\n", err );
+ return -1;
+ }
+
+ // FIXME: This should be hsa instead of hip
+ // AMDCHECK: {{.*}} hip
+ printf( "omp_get_interop_str returned %s\n", interop_fr_name );
+
+ #pragma omp interop destroy(iobj)
+ return 0;
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
The test covers some of the identifier symbols in the interop runtime.
45b7bab
to
54544d9
Compare
The required linked PR got updated to include that fix. Adjusting the test to reflect the correct behavior.
Thanks for creating the PR to add a test for this 😄 Just for my understanding: We add a test to ensure that the strings for AMDGPU are correct. Looking at the already existing test for NVIDIA/CUDA in offload/test/offloading/interop.c, we never check the returned values and basically just try to ensure that the calls don't crash (with one additional test to check if we got a CUDA stream from the OpenMP runtime), right? Should we add similar tests for Intel & NVIDIA as well? |
Just wondering if there's a way to have this test enabled for all plugins and not just AMD (as otherwise we would need the same for each other plugin at least?). Like having another RUN command for NVIDIA with a different prefix for example? Also, I'd suggest to loop through all devices and not just the default device:
|
|
||
int main(int argc, char **argv) { | ||
omp_interop_t iobj = omp_interop_none; | ||
#pragma omp interop init(targetsync : iobj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the usage in this test, I think it should of type target not targetsync:
#pragma omp interop init(targetsync : iobj) | |
#pragma omp interop init(target : iobj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making this change gives me a seg fault during execution
And to run an interop test on AMDGPU while we bring up the functionality required to make
I think we should. And we can include that in this test via nvidia-
Absolutely. I mostly cannot test it easily on other platforms. I can add the |
I'll start a build of #161429 on a machine so that I can provide the strings for NVIDIA. Don't know if Intel would work before #158900 is merged. |
- Added Nvidia run line - Added loop around all devices
The strings for NVIDIA look good: $ clang --version
clang version 22.0.0git (https://github.com/adurang/llvm-project.git 6090c32a072bd97fc24709c91ffe6dde770e0577)
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /p/project1/ccstpa/reuter1/Projects/Sources/LLVM/interop/llvm-project/_build/_install/bin
$ ./bin/clang test.c -fopenmp --offload-arch=sm_90 -frtlib-add-rpath -L$(pwd)/_install/lib/aarch64-unknown-linux-gnu -Wl,-rpath=$(pwd)/_install/lib/aarch64-unknown-linux-gnu
$ ./a.out
omp_get_interop_int returned cuda
omp_get_interop_str returned nvidia
omp_get_interop_str returned cuda
"PluginInterface" error: Failure to synchronize interop object 0x0000aaaaeccff8c0: "generic error code for features unsupported by the device/backend" sync_barrier not supported
omptarget fatal error 0: Interop sync barrier failed for 0xaaaaeccff8c0 object
Aborted (core dumped) The error at the end should be expected if I understood the discussion in the other PRs correctly. Switching to Regarding multi-GPU with different archs, maybe I can check this but I can't guarantee that I get this to work. Edit: Unfortunately not, as the AMD GPU on that system doesn't seem to work correctly at the moment. |
Unfortunately that crashes on AMD. :) |
In that case, we should just keep this as is. As you've said in #161429 (comment), we should implement the missing functionality regardless. So this would just be an additional test that fails for the same reason. |
I think we should then either disable, or XFAIL it on Nvidia though. @jhuber6 just spent some time to make |
That seems odd. I don't think targetsync should work for AMD in the current state of the plugin, and target should be the only one working rn. I feel like the type translation in the wrapper might be incorrect, let me check that. |
@mhalk can you have a look and accept? |
The test covers some of the identifier symbols in the interop runtime.
This test, for now, is to guard against complete breakage, which was the result of the other
interop.c
test not being enabled on AMD and thus, not caught by our buildbots.It can certainly be improved. :)
Depends on #161429 to restore functionality.